[CORE] Apply code checks/format #46999

smuzaffar · 2024-12-18T07:40:46Z

PGO changes ( e.g. using relative source path instead of full path ) broke scram build code-checks rule. So it has not been running properly on PR. code-format was working but only code-checks i.e. clang-tidy did not work as it required correct compiler_command.json with correct source file paths.

Build rules are fixed now, so this PR applies code-checks ( and format) for those files which were merged with proper code-checks

cmsbuild · 2024-12-18T07:41:06Z

cms-bot internal usage

cmsbuild · 2024-12-18T07:45:37Z

-code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-46999/43085

ERROR: Build errors found during clang-tidy run.

/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02868/el8_amd64_gcc12/external/gcc/12.3.1-40d504be6370b5a30e3947a6e575ca28/lib/gcc/x86_64-redhat-linux-gnu/12.3.1/../../../../include/c++/12.3.1/bits/stl_construct.h:115:4: error: no matching function for call to 'construct_at' [clang-diagnostic-error]
  115 |           std::construct_at(__p, std::forward<_Args>(__args)...);
      |           ^~~~~~~~~~~~~~~~~
/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02868/el8_amd64_gcc12/external/gcc/12.3.1-40d504be6370b5a30e3947a6e575ca28/lib/gcc/x86_64-redhat-linux-gnu/12.3.1/../../../../include/c++/12.3.1/bits/alloc_traits.h:635:9: note: in instantiation of function template specialization 'std::_Construct<std::basic_ofstream<char>, const char *, const std::_Ios_Openmode &, edm::service::close_and_delete>' requested here
--
/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02868/el8_amd64_gcc12/external/gcc/12.3.1-40d504be6370b5a30e3947a6e575ca28/lib/gcc/x86_64-redhat-linux-gnu/12.3.1/../../../../include/c++/12.3.1/bits/stl_construct.h:119:25: error: no matching constructor for initialization of 'std::basic_ofstream<char>' [clang-diagnostic-error]
  119 |       ::new((void*)__p) _Tp(std::forward<_Args>(__args)...);
      |                         ^   ~~~~~~~~~~~~~~~~~~~~~~~~~~~
/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02868/el8_amd64_gcc12/external/gcc/12.3.1-40d504be6370b5a30e3947a6e575ca28/lib/gcc/x86_64-redhat-linux-gnu/12.3.1/../../../../include/c++/12.3.1/fstream:795:7: note: candidate constructor not viable: requires at most 2 arguments, but 3 were provided
--
gmake: *** [config/SCRAM/GMake/Makefile.coderules:129: code-checks] Error 2
gmake: *** [There are compilation/build errors. Please see the detail log above.] Error 2

smuzaffar · 2024-12-18T09:59:24Z

FWCore/MessageService/src/ELoutput.cc

@@ -632,7 +633,7 @@ namespace edm {
    }

    void ELoutput::changeFile(const std::string& filename) {
-      os.reset(new std::ofstream(filename.c_str(), std::ios /*_base*/ ::app), close_and_delete());
+      os = std::make_shared<std::ofstream>(filename.c_str(), std::ios /*_base*/ ::app, close_and_delete());


@makortel , as std::make_shared does not allow a custom deleter. so should I revert this change and add // NOLINT ?

as std::make_shared does not allow a custom deleter. so should I revert this change and add // NOLINT ?

Yes, this change should be reverted.

So clang-tidy made a change that it then failed to process? :(

makortel · 2024-12-18T15:48:35Z

FWCore/Framework/src/EventPrincipal.cc

@@ -214,7 +213,7 @@ namespace edm {
                                 std::optional<ProductProvenance> productProvenance) const {
    assert(!bd.produced());
    if (productProvenance) {
-      productProvenanceRetrieverPtr()->insertIntoSet(std::move(*productProvenance));
+      productProvenanceRetrieverPtr()->insertIntoSet(*productProvenance);


Just to note for future reference, ProductProvenance is as efficient to copy as it is to move.

smuzaffar · 2024-12-18T21:39:50Z

please test

cmsbuild · 2024-12-18T21:42:39Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-46999/43108

cmsbuild · 2024-12-18T21:43:04Z

A new Pull Request was created by @smuzaffar for master.

It involves the following packages:

DataFormats/Provenance (core)
FWCore/Framework (core)
FWCore/MessageService (core)
FWCore/Modules (core)
FWCore/Services (core)
IOPool/Streamer (core)
PerfTools/AllocMonitorPreload (core)
Utilities/XrdAdaptor (core)

@Dr15Jones, @makortel, @smuzaffar can you please review it and eventually sign? Thanks.
@felicepantaleo, @fwyzard, @makortel, @missirol, @mmusich, @rovere, @wddgit this is something you requested to watch as well.
@antoniovilela, @mandrenguyen, @rappoccio, @sextonkennedy you are the release manager for this.

cms-bot commands are listed here

Dr15Jones · 2024-12-18T21:55:36Z

PerfTools/AllocMonitorPreload/src/memory_proxies.cc

@@ -148,7 +148,7 @@ namespace {
    }
  };

-  static ThreadTracker& getTracker() {


I don't like this code change.

Isn't this all inside an anonymous namespace anyway ?

Isn't this all inside an anonymous namespace anyway ?

yes @fwyzard. clang-tidy removed the static due to exact same reason (https://clang.llvm.org/extra/clang-tidy/checks/readability/static-definition-in-anonymous-namespace.html)

warning: 'getTracker' is a static definition in anonymous namespace; static is redundant here [readability-static-definition-in-anonymous-namespace]

I should have looked closer. I had thought the function was actually a member function of a class, not a free function.

cmsbuild · 2024-12-18T23:37:29Z

+1

Size: This PR adds an extra 24KB to repository
Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-39712b/43557/summary.html
COMMIT: c0045dc
CMSSW: CMSSW_15_0_X_2024-12-18-1100/el8_amd64_gcc12
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmssw/46999/43557/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

You potentially added 4 lines to the logs
Reco comparison results: 9 differences found in the comparisons
DQMHistoTests: Total files compared: 46
DQMHistoTests: Total histograms compared: 3510017
DQMHistoTests: Total failures: 451
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3509546
DQMHistoTests: Total skipped: 20
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 45 files compared)
Checked 202 log files, 172 edm output root files, 46 DQM output files
TriggerResults: no differences found

makortel · 2024-12-19T14:12:32Z

Comparison differences are related to #46416

makortel · 2024-12-19T14:12:35Z

+core

cmsbuild · 2024-12-19T14:12:55Z

This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @sextonkennedy, @mandrenguyen, @antoniovilela, @rappoccio (and backports should be raised in the release meeting by the corresponding L2)

mandrenguyen · 2024-12-20T09:42:22Z

+1

cmsbuild added this to the CMSSW_15_0_X milestone Dec 18, 2024

cmsbuild added core-pending pending-signatures tests-pending orp-pending code-checks-pending labels Dec 18, 2024

cmsbuild added code-checks-rejected and removed code-checks-pending labels Dec 18, 2024

smuzaffar commented Dec 18, 2024

View reviewed changes

makortel reviewed Dec 18, 2024

View reviewed changes

[CORE] Apply code checks/format

c0045dc

smuzaffar force-pushed the core-code-checks1 branch from ea26d4a to c0045dc Compare December 18, 2024 21:38

cmsbuild added code-checks-pending and removed code-checks-rejected labels Dec 18, 2024

cmsbuild added tests-started and removed tests-pending labels Dec 18, 2024

cmsbuild added code-checks-approved and removed code-checks-pending labels Dec 18, 2024

Dr15Jones reviewed Dec 18, 2024

View reviewed changes

cmsbuild added tests-approved and removed tests-started labels Dec 18, 2024

cmsbuild removed the core-pending label Dec 19, 2024

cmsbuild added core-approved fully-signed and removed pending-signatures labels Dec 19, 2024

cmsbuild added orp-approved and removed orp-pending labels Dec 20, 2024

cmsbuild merged commit 830e6ae into cms-sw:master Dec 20, 2024
11 checks passed

This was referenced Dec 20, 2024

clean-up unused inclusions in HLT paths in Phase2 HLT menu #46976

Merged

[CLANG]Update llvm to tag 19.1.6 cms-sw/cmsdist#9585

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CORE] Apply code checks/format #46999

[CORE] Apply code checks/format #46999

smuzaffar commented Dec 18, 2024

cmsbuild commented Dec 18, 2024 •

edited

Loading

cmsbuild commented Dec 18, 2024

smuzaffar Dec 18, 2024

makortel Dec 18, 2024

makortel Dec 18, 2024 •

edited

Loading

makortel Dec 18, 2024

smuzaffar commented Dec 18, 2024

cmsbuild commented Dec 18, 2024

cmsbuild commented Dec 18, 2024

Dr15Jones Dec 18, 2024

fwyzard Dec 18, 2024

smuzaffar Dec 19, 2024 •

edited

Loading

Dr15Jones Dec 19, 2024

cmsbuild commented Dec 18, 2024

makortel commented Dec 19, 2024

makortel commented Dec 19, 2024

cmsbuild commented Dec 19, 2024

mandrenguyen commented Dec 20, 2024

[CORE] Apply code checks/format #46999

[CORE] Apply code checks/format #46999

Conversation

smuzaffar commented Dec 18, 2024

cmsbuild commented Dec 18, 2024 • edited Loading

cmsbuild commented Dec 18, 2024

smuzaffar Dec 18, 2024

Choose a reason for hiding this comment

makortel Dec 18, 2024

Choose a reason for hiding this comment

makortel Dec 18, 2024 • edited Loading

Choose a reason for hiding this comment

makortel Dec 18, 2024

Choose a reason for hiding this comment

smuzaffar commented Dec 18, 2024

cmsbuild commented Dec 18, 2024

cmsbuild commented Dec 18, 2024

Dr15Jones Dec 18, 2024

Choose a reason for hiding this comment

fwyzard Dec 18, 2024

Choose a reason for hiding this comment

smuzaffar Dec 19, 2024 • edited Loading

Choose a reason for hiding this comment

Dr15Jones Dec 19, 2024

Choose a reason for hiding this comment

cmsbuild commented Dec 18, 2024

Comparison Summary

makortel commented Dec 19, 2024

makortel commented Dec 19, 2024

cmsbuild commented Dec 19, 2024

mandrenguyen commented Dec 20, 2024

cmsbuild commented Dec 18, 2024 •

edited

Loading

makortel Dec 18, 2024 •

edited

Loading

smuzaffar Dec 19, 2024 •

edited

Loading